Skip to content

Conversation

karuppaiyak
Copy link

No description provided.

@karuppaiyak karuppaiyak requested a review from a team as a code owner September 4, 2025 12:12
hgfell683 and others added 4 commits September 4, 2025 11:29
* RDKEMW-7169 - Gtest Add missing empty files

* RDKEMW-7169 - Gtest Add missing empty files

* RDKEMW-7169 - Gtest Add missing empty files

* RDKEMW-7169 - Gtest Add missing empty files

* RDKEMW-7169 - Gtest Add missing empty files

---------

Co-authored-by: Srikanth <[email protected]>
1.4.6 release 1.4.6
@skamath skamath requested a review from Copilot September 5, 2025 05:52
Copilot

This comment was marked as outdated.

Copy link
Contributor

@skamath skamath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to remove dsMgr.h and other unused header from all plugins.

@@ -418,6 +425,11 @@ namespace WPEFramework
{
//TODO(MROLLINS) this is probably per process so we either need to be running in our own process or be carefull no other plugin is calling it
device::Manager::Initialize();

#ifndef IO_HCEC_ENABLE_IARM
device::Host::getInstance().Register(this, "WPE[HdmiCecSource]");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to truncate clientName to less than 11 chars so that, small string optimization is used, take care at all instances. Also I see in other PR's WPE:: convention is used.

InitializeIARM();
#else
device::Host::getInstance().Register(this, "WPE[HdmiCecSink]");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is device::manager::Initialize() called before register ?

@skamath skamath requested a review from Copilot September 9, 2025 12:01
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces HdmiCecSource and HdmiCecSink changes that provide alternative implementations for HDMI event handling by adding support for direct device host interfaces alongside existing IARM-based implementations.

Key changes include:

  • Addition of conditional compilation support via IO_HCEC_ENABLE_IARM preprocessor flag
  • Implementation of device::Host event interface callbacks as alternatives to IARM event handlers
  • Enhanced error handling and initialization logic for device manager components

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
HdmiCecSource/HdmiCecSourceImplementation.h Adds IDisplayDeviceEvents interface inheritance and OnDisplayHDMIHotPlug declaration
HdmiCecSource/HdmiCecSourceImplementation.cpp Implements conditional IARM vs device::Host event handling with preprocessor guards
HdmiCecSink/HdmiCecSink.h Adds IHdmiInEvents interface inheritance and OnHdmiInEventHotPlug declaration
HdmiCecSink/HdmiCecSink.cpp Refactors initialization with device manager setup and conditional IARM/device::Host implementations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 3648 to 3652
#if 0 //just to verify compilation
device::HdmiInput::getInstance().getHDMIARCPortId(&portId);
#else
portId = 0;
#endif
Copy link
Preview

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented-out code with #if 0 suggests unfinished implementation. This hardcoded portId = 0 assignment bypasses the actual device API call and could lead to incorrect behavior in production. Either implement the proper API call or add a TODO comment explaining why this is temporarily disabled.

Suggested change
#if 0 //just to verify compilation
device::HdmiInput::getInstance().getHDMIARCPortId(&portId);
#else
portId = 0;
#endif
// TODO: Replace hardcoded portId assignment with actual device API call when available.
// device::HdmiInput::getInstance().getHDMIARCPortId(&portId);
portId = 0;

Copilot uses AI. Check for mistakes.

LOGINFO("Number of Inputs [%d] \n", hdmiInput.numHdmiInputs );
m_numofHdmiInput = hdmiInput.numHdmiInputs;
}else{
LOGINFO("Not able to get Numebr of inputs so defaulting to 3 \n");
Copy link
Preview

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Numebr" is misspelled and should be "Number".

Suggested change
LOGINFO("Not able to get Numebr of inputs so defaulting to 3 \n");
LOGINFO("Not able to get Number of inputs so defaulting to 3 \n");

Copilot uses AI. Check for mistakes.

@skamath skamath requested a review from Copilot September 17, 2025 08:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

isAnyPortConnected = true;
}
LOGINFO("update Port Status [%d] \n", i);
hdmiInputs[i].update(device::HdmiInput::getInstance().isPortConnected(i));
Copy link
Preview

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isPortConnected call could throw an exception if the port index is invalid or the HdmiInput instance is not properly initialized. Consider adding bounds checking and exception handling around this call.

Suggested change
hdmiInputs[i].update(device::HdmiInput::getInstance().isPortConnected(i));
bool isConnected = false;
try {
// If HdmiInput provides a method to get the number of ports, use it for bounds checking
int numPorts = device::HdmiInput::getInstance().getNumberOfPorts();
if (i >= 0 && i < numPorts) {
isConnected = device::HdmiInput::getInstance().isPortConnected(i);
} else {
LOGERR("Port index [%d] is out of range [0, %d)", i, numPorts);
}
} catch (const std::exception& ex) {
LOGERR("Exception in isPortConnected(%d): %s", i, ex.what());
} catch (...) {
LOGERR("Unknown exception in isPortConnected(%d)", i);
}
hdmiInputs[i].update(isConnected);

Copilot uses AI. Check for mistakes.

do {
usleep(50000); // Sleep for 50ms before retrying
portId = -1;
device::HdmiInput::getInstance().getHDMIARCPortId(&portId);
Copy link
Preview

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getHDMIARCPortId call lacks error handling. If this method throws an exception or fails, the retry logic may not work as expected. Consider wrapping this in a try-catch block to handle potential failures gracefully.

Suggested change
device::HdmiInput::getInstance().getHDMIARCPortId(&portId);
try {
device::HdmiInput::getInstance().getHDMIARCPortId(&portId);
} catch (const std::exception& ex) {
LOGERR("Exception in getHDMIARCPortId: %s (retry count[%d])", ex.what(), retryCount);
// Continue to next retry
} catch (...) {
LOGERR("Unknown exception in getHDMIARCPortId (retry count[%d])", retryCount);
// Continue to next retry
}

Copilot uses AI. Check for mistakes.

LOGINFO(" Add to vector [%d] \n", i);
hdmiInputs.push_back(hdmiPort);
}
m_numofHdmiInput = device::HdmiInput::getInstance().getNumberOfInputs();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can throw exception, see

@karuppaiyak karuppaiyak force-pushed the feature/RDKEMW-6163-c1 branch from bc803aa to 95d7e4f Compare September 17, 2025 15:05
@CLAassistant
Copy link

CLAassistant commented Sep 17, 2025

CLA assistant check
All committers have signed the CLA.

@karuppaiyak karuppaiyak force-pushed the feature/RDKEMW-6163-c1 branch 8 times, most recently from e34b4c5 to 73a1113 Compare September 19, 2025 13:01
@karuppaiyak karuppaiyak force-pushed the feature/RDKEMW-6163-c1 branch from 73a1113 to 0951d07 Compare September 19, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants